-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add shading tests #390
Add shading tests #390
Conversation
@@ -34,6 +34,10 @@ | |||
<groupId>org.glassfish.jersey.connectors</groupId> | |||
<artifactId>*</artifactId> | |||
</exclusion> | |||
<exclusion> | |||
<groupId>log4j</groupId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a common rule not to ship logging implementation with libraries. I have no idea why @docker-java does that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -24,7 +24,6 @@ | |||
import java.util.Date; | |||
import java.util.concurrent.TimeUnit; | |||
|
|||
import static com.google.common.base.Preconditions.checkState; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since it's the only place where we use Guava, I removed it from selenium
module and inlined the implementation of checkState
, so that we don't have to shade guava into the module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 good idea - I thought it was more widely used but it's good to remove it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good addition
@@ -87,8 +86,11 @@ protected Integer getLivenessCheckPort() { | |||
@Override | |||
protected void configure() { | |||
|
|||
checkState(desiredCapabilities != null); | |||
if (! customImageNameIsSet) { | |||
if(desiredCapabilities == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (desiredCapabilities == null) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
er?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added space between if
and (
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I see. Will fix it now
|
||
@BeforeClass | ||
public static void setUp() throws Exception { | ||
Path path = Paths.get("..", "..", "core", "target", "testcontainers-0-SNAPSHOT.jar"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is prone to refactor of locations.
How about you copy this jar to this project target directory using copy-dependencies goal of maven-dependencies-plugin? You can add it in the pom.xml of this project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not very IDE-friendly I'm afraid.
Test will fail if we refactor the locations => should be fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please elaborate why is it not IDE friendly?
I though one of Maven projects best practice is to avoid depending on explicit locations, but depends on artifacts. When you use copy dependencies you alleviate your self from that since you use GAV and not relative paths. You are free then to place core where ever you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We run tests from IDE, and would like to keep that. Doing something in Maven means that we have to run the tests with Maven and only Maven.
Personally, I would prefer to use Gradle and "delegate build to Gradle" feature of IntelliJ, but we use Maven ATM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this test you rely on the fact that someone ran mvn install on the core project right? Once he has done that you can the test from the IDE.
I'm saying the same thing - someone must run mvn install on the core project and on the project it self. Then you run the test from the IDE - same.
So both rely on manual mvn operation.
Am I missing something here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then how does it matter where the jar file is? Again, the test will fail if we change paths.
Doing "copy dependency" might lead to the dependency copied from Maven's local repo => more magical and error-prone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's going to be pretty obvious why this has broken if someone ever manages to break it, so I think we'll be OK with this.
public static void setUp() throws Exception { | ||
Path path = Paths.get("..", "..", "core", "target", "testcontainers-0-SNAPSHOT.jar"); | ||
|
||
fileSystem = FileSystems.newFileSystem(URI.create("jar:" + path.toUri()), emptyMap()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really cool trick!
); | ||
|
||
assertThatFileList(root.resolve("META-INF").resolve("native")).containsOnly( | ||
"liborg-testcontainers-shaded-netty-transport-native-epoll.so" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funny that we shade something which is architecture specific - I mean this might not work for Mac or Windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not used where not supposed to be :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean? Docker Java Client doesn't use Netty communicate over TCP to Docker daemon? Does it do it using this native library or plain nio?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We choose different strategies on different platforms, also Netty has a mechanism where they optionally load epoll library where it's supported ( Linux )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing that this will have to change when merged with #393 and netty-transport-native-kqueue
comes in. NBD though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great - awesome work! This should help fix real pain for users, and will help us systematically solve future shading issues as well. +💯!
|
||
@BeforeClass | ||
public static void setUp() throws Exception { | ||
Path path = Paths.get("..", "..", "core", "target", "testcontainers-0-SNAPSHOT.jar"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's going to be pretty obvious why this has broken if someone ever manages to break it, so I think we'll be OK with this.
AFAICT you've resolved all discussions with @asafm so, from my perspective, this should be good to merge! |
Shading is a hot topic, and usually painful :D
I added a few tests of our final jar to make sure that our shading configuration is correct.
Also, during this PR, I revisited a few dependencies, removed shading from
jdbc
module (no idea why it was there :D)N.B! run
mvn -DskipTests -pl core clean package
before running the test from IDE